DB and HTTP semantic convention stability migration for Redis instrumentation#4370
DB and HTTP semantic convention stability migration for Redis instrumentation#4370tammy-baylis-swi wants to merge 9 commits intoopen-telemetry:mainfrom
Conversation
| pluggy==1.6.0 | ||
| py-cpuinfo==9.0.0 | ||
| pytest==7.4.4 | ||
| pytest-asyncio==0.23.5 |
There was a problem hiding this comment.
Added in 1c7e3cc so Python 3.9 tests pass, else RuntimeError: There is no current event loop in thread 'MainThread'.
MikeGoldsmith
left a comment
MikeGoldsmith
left a comment
There was a problem hiding this comment.
Looks good - thanks @tammy-baylis-swi.
Left some queries about some semconv keys and a couple of suggestions.
| ) | ||
|
|
||
| db = conn_kwargs.get("db", 0) | ||
| attributes[DB_REDIS_DATABASE_INDEX] = db |
There was a problem hiding this comment.
I think db.redis.database_index became db.namespace - do we have a set helper that can use old/new/dual write for these?
| _set_http_net_peer_name_client( | ||
| attributes, conn_kwargs.get("path", ""), http_sem_conv_opt_in_mode | ||
| ) | ||
| attributes[NET_TRANSPORT] = NetTransportValues.OTHER.value |
There was a problem hiding this comment.
I think net.transport has been deprecated - should we drop this in stable mode?
There was a problem hiding this comment.
yeah. maybe just set if _report_old()
| conn_kwargs.get("port", 6379), | ||
| http_sem_conv_opt_in_mode, | ||
| ) | ||
| attributes[NET_TRANSPORT] = NetTransportValues.IP_TCP.value |
| self.assertEqual(span.status.status_code, trace.StatusCode.UNSET) | ||
|
|
||
|
|
||
| class TestRedisSemconvConfiguration(TestRedis): |
There was a problem hiding this comment.
Do we need to inherit TestRedis or could we use TestBase? I think we'll re-run all the stuff in TestRedis like this.
I think we're missing the async tests for _async_traced_execute_factory and _async_traced_execute_pipeline_factory too.
| response_hook: ResponseHook | None = None, | ||
| ): | ||
| # Get semconv opt-in modes for database and HTTP signal types | ||
| _OpenTelemetrySemanticConventionStability._initialize() |
There was a problem hiding this comment.
These are duplicated across all four factories, should we add a helper?
3 participants
Description
Add Redis instrumentor support for when
OTEL_SEMCONV_STABILITY_OPT_INincludesEDIT: This uses now-merged updates from SQLAlchemy PR, no conflicts 🙂
This duplicates all_semconvhelper updates in #4110 at 02adc40Fixes #2930, #2885
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.